Repository navigation
fix(modelserving): preserve semantic revisions across upgrades - #1799
volcano-sh-bot merged 2 commits into
Conversation
|
/kind bug |
|
How should this compose with #1767, which switches to scheduler/plugin-aware revisions and intentionally rolls legacy workloads on upgrade? |
12d1447 to
eb6b22a
Compare
aff1291 to
7f9be97
Compare
|
/cherry-pick release-1.1 |
|
@LiZhenCheng9527: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
7f9be97 to
7ea1d7d
Compare
Use canonical workload snapshots and trusted semantic history for ModelServing and Role identities, local recovery, readiness, PodGroup requirements and in-place updates. Integrate resolved identities with coordinated rollout and partial creation. Preserve replica baseline metadata, live history references, headless rendering semantics and existing rollout policies. Signed-off-by: Min Chen <vanderchen@outlook.com>
Validate effective scheduler and Role name sets on UPDATE while preserving upstream plugin and coordination validation. Allow equivalent representations and cover zero-replica workloads and AdmissionReview boundaries. Signed-off-by: Min Chen <vanderchen@outlook.com>
7ea1d7d to
37bf0c7
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiZhenCheng9527 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@LiZhenCheng9527: new pull request created: #1968 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Upgrading only the Kthena controller can trigger a one-time rollout of already running ModelServing workloads even when the user has not changed the ModelServing Spec. On the first reconciliation after the upgrade, the new controller can calculate a different revision for the same persisted Role/Pod template. Existing Pods still carry the old revision labels, so they are classified as outdated and replaced. The workload stabilizes after every Pod carries the new revision, making the symptom look like a one-off rollout during controller upgrade.
The root cause is that
mainhashes the in-memory Go representation ofspec.template.roleswithdump.ForHash. A Kubernetes dependency or CRD Go type update can add an optional nil field to a nested struct. That changes the typed dump and hash even when the serialized Role/Pod template is unchanged.This PR:
omitemptyno longer change the serialized hash;go.mod,go.sum, APIs, generated code, Helm CRDs, or CRD schemas.Revision and rollout field scope:
spec.template.roles[].nameentryTemplate.metadataandentryTemplate.specworkerReplicasand activeworkerTemplatepluginsschedulerNamevolcanoat admission, whereas explicit empty means Kubernetesdefault-scheduler.replicasmaxUnavailable,maxSurge,partitionspec.replicasandspec.rolloutStrategyrecoveryPolicyandrevisionHistoryLimitrestartGracePeriodSeconds,gangPolicy,networkTopologyschedulerName;GROUP_SIZE,ENTRY_ADDRESS,WORKER_INDEXenvironment entriesPlugin comparison uses configuration inputs, not hook output. Arbitrary Pod API defaults are not stripped: hooks can observe pre-admission input.
Migration boundary: legacy snapshots have no old plugin configuration. The first migration uses current plugins as the compatibility baseline and does not force a rollout. A plugin edit simultaneous with that migration cannot be detected. Later plugin edits are detected against the durable baseline, including after restart. An annotation on the original revision records the established baseline; losing that baseline blocks reconciliation instead of silently rebuilding it from current plugins.
Which issue(s) this PR fixes:
Fixes #1803
Bug evidence (required for bug-related PRs):
Trigger:
k8s.io/api v0.34.2.k8s.io/api v0.35.3; do not change the ModelServing Spec.PodSpeccontains the optional nilWorkloadReffield. Serialized Role JSON remains identical, butdump.ForHashproduces a different revision.syncModelServingpasses the new revision into rolling-update handling. Existing Pods have the legacy revision label and are treated as outdated, triggering one unnecessary rollout.Representative workload:
Cross-version hash probe using the same Role JSON:
k8s.io/api v0.34.2WorkloadRef655f5cbc6fk8s.io/api v0.35.3WorkloadRef: nil7d97c697b8Production path at base commit
d5f99e10a1996714bb29228cfa5af451c5305639:Revisionhashes typed values throughdump.ForHash.ModelServingRevisionandCalRoleTemplateHashpass Role structs directly to that function.syncModelServingcalculates the revision on every reconciliation and passes it to rolling-update handling.This issue was found through source review and reproduced with the isolated dependency probe and Kind controller-upgrade path; no production incident log is available.
Verification:
go test ./pkg/model-serving-controller/... -count=1go test $(go list ./... | grep -v /e2e)make -o generate test(coverage gate; generation intentionally omitted because no API/schema changes are made andgeneraterunsgo mod tidy)go vet ./pkg/model-serving-controller/...go test -racecoverage for revision equality, entry points, baseline migration, scheduler admission, missing-history observation and partition recovery;omitemptyfield changes the legacy typed hash but not the serialized hash;ModelServingRevision,RoleRevisionHashand legacyCalRoleTemplateHash, repeat calls, and verify the input Spec is unchanged;dump.ForHashrevision is reused after switching to serialized hashing;884928deretained all 11 original Pod UIDs, status revision identities and original ControllerRevision UID/Data across both rollout strategies; additional baseline objects are expected;Representative Kind evidence from the unchanged-spec upgrade (same UID before/after):
migrate-sg-0-decode-0-07ff0cefc-1900-4c56-95e4-f103c9ff1ed868c7587c89migrate-role-0-decode-0-045683189-81a0-4e2a-a8ea-7c60670c153c68c7587c89During deliberate baseline-loss injection, the controller retried with:
Pod UIDs stayed unchanged until the saved baseline was restored, after which the pending real plugin edit proceeded only outside partition.
Verification caveats (not reported as passing):
make lintis blocked by the repository-pinned golangci-lint v1.64.8 export-data reader under host Go 1.27.1 (export data version 4 is greater than maximum supported version 2). No dependency or lint configuration workaround is included.-racedetects the existing unsynchronizedinitialSyncread inmodel_serving_controller_harness_test.go:95versus the write insyncAll. Reproduced on unmodifiedupstream/mainat4f293e39; focused revision tests pass with-race. This unrelated race is not changed here.Special notes for your reviewer:
Kubernetes ControllerRevision users hash serialized revision data and use semantic comparison of reconstructed inputs where appropriate. This PR applies that two-layer approach to the same canonical projections used for ModelServing and Role decisions.
The hash algorithm intentionally changes from typed
dump.ForHashto serialized bytes. The semantic history fallback is therefore required to absorb this migration without rolling existing workloads. If the referenced ControllerRevision is missing, malformed, or owned by another resource, template-driven rollout remains blocked and retried rather than assuming equality.Parts of the full-snapshot recovery and Role projection follow #1767 (reviewed at
b8da6b8868f62a1fd52953dd20aaf7f1af1a92f8). This is not a wholesale import:status.revisionReferencesor other CRD/API additions are introduced.This PR was written in part with the assistance of generative AI.
Does this PR introduce a user-facing change?: